Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

fix induced_subgraph indexing with vector of Bools #1573

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bkamins
Copy link
Contributor

@bkamins bkamins commented Jul 5, 2021

Currently both getindex and induced_subgraph handle AbstractVector{Bool} argument in a way inconsistent with Julia Base rules.

bkamins added 2 commits July 5, 2021 18:39
Currently both `getindex` and `induced_subgraph` handle `AbstractVector{Bool}` argument in a way inconsistent with Julia Base rules.
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #1573 (37c6aca) into master (712b8d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 37c6aca differs from pull request most recent head 5032b51. Consider uploading reports for the commit 5032b51 to get more accurate results

@@           Coverage Diff           @@
##           master    #1573   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         106      106           
  Lines        5551     5554    +3     
=======================================
+ Hits         5520     5523    +3     
  Misses         31       31           

@sbromberger
Copy link
Owner

Sorry - what's the specific problem here?

@sbromberger
Copy link
Owner

It seems as if you're trying to use a bitvec / vector of boolean where true values indicate inclusion in the induced subgraph. That's not (currently) how it works: the vlist is a list of vertex indices.

@bkamins
Copy link
Contributor Author

bkamins commented Jul 5, 2021

I agree it does not work this way now. That is why I propose to change it (indeed - docstrings should be updated then; I can fix it if you agree to the proposal).

  1. I think what I propose would be often useful (when you have a predicate that you apply to a vertex to decide if it should stay or not stay)
  2. you cannot use Bool as index now anyway (so the change is not breaking), see:
julia> g = SimpleGraph(2)
{2, 0} undirected simple Int64 graph

julia> g[[true]]
ERROR: ArgumentError: invalid index: true of type Bool

julia> g[[true, true]]
ERROR: ArgumentError: Vertices in subgraph list must be unique

  1. You call this function in getindex and current getindex definition is inconsistent with the definition in Julia Base as commented in the original comment. Consider the following examples:
julia> g = SimpleGraph(2)
{2, 0} undirected simple Int64 graph

julia> g[[true, false]]
ERROR: InexactError: Bool(2)

julia> x = [1, 2]
2-element Vector{Int64}:
 1
 2

julia> x[[true, false]]
1-element Vector{Int64}:
 1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants